Added enhancements on top of Calling Server preview 1 by navali-msft · Pull Request #21686 · Azure/azure-sdk-for-net

navali-msft · 2021-06-08T08:48:38Z

All SDK Contribution checklist:

This checklist is used to make sure that common guidelines for a pull request are followed.

Please open PR in Draft mode if it is:
- Work in progress or not intended to be merged.
- Encountering multiple pipeline failures and working on fixes.
If an SDK is being regenerated based on a new swagger spec, a link to the pull request containing these swagger spec changes has been included above.
I have read the contribution guidelines.
The pull request does not introduce breaking changes.

General Guidelines and Best Practices

Title of the pull request is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

Pull request includes test coverage for the included changes.

SDK Generation Guidelines

The generate.cmd file for the SDK has been updated with the version of AutoRest, as well as the commitid of your swagger spec or link to the swagger spec, used to generate the code. (Track 2 only)
The *.csproj and AssemblyInfo.cs files have been updated with the new version of the SDK. Please double check nuget.org current release version.

Additional management plane SDK specific contribution checklist:

Note: Only applies to Microsoft.Azure.Management.[RP] or Azure.ResourceManager.[RP]

Include updated management metadata.
Update AzureRP.props to add/remove version info to maintain up to date API versions.

Management plane SDK Troubleshooting

If this is very first SDK for a services and you are adding new service folders directly under /SDK, please add new service label and/or contact assigned reviewer.
If the check fails at the Verify Code Generation step, please ensure:
- Do not modify any code in generated folders.
- Do not selectively include/remove generated files in the PR.
- Do use generate.ps1/cmd to generate this PR instead of calling autorest directly.
  Please pay attention to the @microsoft.csharp version output after running generate.ps1. If it is lower than current released version (2.3.82), please run it again as it should pull down the latest version.
Note: We have recently updated the PSH module called by generate.ps1 to emit additional data. This would help reduce/eliminate the Code Verification check error. Please run following command:
```
  `dotnet msbuild eng/mgmt.proj /t:Util /p:UtilityName=InstallPsModules`
```

Old outstanding PR cleanup

Please note:
If PRs (including draft) has been out for more than 60 days and there are no responses from our query or followups, they will be closed to maintain a concise list for our reviewers.

…tity with alternateCallerId, other fixes

check-enforcer · 2021-06-08T17:02:31Z

This pull request is protected by Check Enforcer.

What is Check Enforcer?

Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass.

Why am I getting this message?

You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged.

What should I do now?

If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows:
/check-enforcer evaluate
Typically evaulation only takes a few seconds. If you know that your pull request is not covered by a pipeline and this is expected you can override Check Enforcer using the following command:
/check-enforcer override
Note that using the override command triggers alerts so that follow-up investigations can occur (PRs still need to be approved as normal).

What if I am onboarding a new service?

Often, new services do not have validation pipelines associated with them. In order to bootstrap pipelines for a new service, please perform following steps:

For data-plane/track 2 SDKs Issue the following command as a pull request comment:

/azp run prepare-pipelines
This will run a pipeline that analyzes the source tree and creates the pipelines necessary to build and validate your pull request. Once the pipeline has been created you can trigger the pipeline using the following comment:
/azp run net - [service] - ci

For track 1 management-plane SDKs

Please open a separate PR and to your service SDK path in this file. Once that PR has been merged, you can re-run the pipeline to trigger the verification.

zihzhan-msft · 2021-06-08T18:23:07Z

        /// </summary>
-        /// <returns>The instrumented <see cref="CallClient" />.</returns>
-        protected CallClient CreateInstrumentedCallingServerClient()
+        /// <returns>The instrumented <see cref="CallConnection" />.</returns>


I think the return should be CallingServerClient

This is fixed.

…s and live tests

ajaych-ms · 2021-06-09T00:46:52Z

 To get started you'll need a Communication Service Resource.  See [README][README] for prerequisites and instructions.

-## Creating an `ServerCallingClient`
+## Creating an `CallingServerClient`


an

a #Resolved

I still see an?

Still see "an" as well

Addressed now.

still see an :)

No more "an"

ajaych-ms · 2021-06-09T00:48:58Z

    options: createCallOption // The options for creating a call.
    );
-Console.WriteLine($"Call Leg id: {createCallResponse.CallLegId}");
+Console.WriteLine($"Call connection id: {callConnection.Value.CallConnectionId}");


.Value

why do we need .Value?

Apis are returning Response object which kind of wrapped http response object. The Value property contain the return value.

then variable should be named differently - something like callConnectionResponse

…er-preview1-enhancement

* Clean up clients comments. * Update Snippet and Export-API.

…github.com/Azure/azure-sdk-for-net into feature/callingServer-preview1-enhancement

* Update Snippet and Export-API. * remove calloption override in servercall client. * Nit for params and arguments handling. * Merge Downloader updates. * Merge Downloader updates test cases. * Update AutoGen Content.

RezaJooyandeh · 2021-06-09T01:30:22Z

@@ -46,9 +46,9 @@ protected async Task SleepIfNotInPlaybackModeAsync()
                await Task.Delay(10000);


Why is there a delay here?

There are few Tests in Record mode required human interaction(i.e. pick up the call to let the call in established state, so that further Api operations can be performed).
Alternative ways is using debug break point.

👍. You can renaming the method then to WaitToPickUpPhoneInPlaybackModeAsync to make it more clear

…d of Response), fixes based on comments (#21790)

* Update Snippet and Export-API. * remove calloption override in servercall client. * Nit for params and arguments handling. * Merge Downloader updates. * Merge Downloader updates test cases. * Update AutoGen Content. * Nit clean on Unit Tests. * Updating and renaming.

InviteParticipants --> AddParticipant InviteParticipantResultEvent --> AddParticipantResultEvent CallModality --> MediaType CommunicationParticipant --> CallParticipant Id in PlayAudioResult & CancelAllMediaOperations is now called OperationId CallConnectionState now has {incoming, connecting, connected, disconnecting, disconnected} CallRecordingStateResult ---> CallRecording... by Naveed Ali CallRecordingStateResult ---> CallRecordingProperties

…github.com/Azure/azure-sdk-for-net into feature/callingServer-preview1-enhancement

…gger path (#21870) * Added AddParticipantResult, Re-added communication error, updated swagger path * Fixed unit tests * Updated netstandard

* Update Snippet and Export-API. * remove calloption override in servercall client. * Nit for params and arguments handling. * Merge Downloader updates. * Merge Downloader updates test cases. * Update AutoGen Content. * Nit clean on Unit Tests. * Updating and renaming. * Test update. * Add Call Connection Live Tests. * Add ServerCall Live Tests * Increase Test Coverage. * Reenable sample tests. * Address comments. * Point to swagger file in master branch.

* Update Snippet and Export-API. * remove calloption override in servercall client. * Nit for params and arguments handling. * Merge Downloader updates. * Merge Downloader updates test cases. * Update AutoGen Content. * Nit clean on Unit Tests. * Updating and renaming. * Test update. * Add Call Connection Live Tests. * Add ServerCall Live Tests * Increase Test Coverage. * Reenable sample tests. * Address comments. * Point to swagger file in master branch. * Nit fix. * Nit changes based on Azure boarder review feedback.

RezaJooyandeh · 2021-06-18T01:19:52Z

+    /// Calling server model factory
+    /// </summary>
+    [CodeGenModel("AzureCommunicationServicesModelFactory")]
+    internal partial class CallingServerModelFactory


this probably should be moved to .Models namespace and also should it be public so developers can instantiate the models that do not have public constructor.
CC: @minnieliu

Yup, agreed, good catch Reza!

Is this a pattern followed by other modalities? So, far we don't seem to have this need for apps

Yes. See this for more details: https://azure.github.io/azure-sdk/dotnet_introduction.html#dotnet-mocking

minnieliu · 2021-06-18T02:31:33Z

  <PropertyGroup>
    <Description>
-      This client library enables working with the Microsoft Azure Communication Service Calling Server APIs.
+      This client library enables working with the Microsoft Azure Communication CallingServer service.


Can we rename to Calling Server, two separate words?

We prefer using "CallingServer" which was "Calling Server" to keep it consistent within SDK library and API

minnieliu · 2021-06-18T02:34:59Z

            /// </summary>
 #pragma warning disable CA1707 // Identifiers should not contain underscores
-            V2021_04_15_Preview1 = 0
+            V2021_06_15_Preview = 1


why is the enum starting with 1 but the 2021_04_15 version is removed?

This number started from 1 was per Azure boarder team suggest, they seems reserve 0 for other usage. CC @navali-msft

"2021_04_15" was a "private review" version. There are plenty of major schema changes on the swagger and since we are not public preview yet, we decide to have "V2021_06_15_Preview" as our first public version.

please see https://azure.github.io/azure-sdk/dotnet_introduction.html#dotnet-versioning-select-api-version

minnieliu · 2021-06-18T02:36:30Z

+        /// <param name="identifier"> The communication identifier. </param>
+        /// <param name="participantId"> Participant Id. </param>
+        /// <param name="isMuted"> Is participant muted. </param>
+        public CallParticipant(CommunicationIdentifier identifier, string participantId, bool? isMuted)


should isMuted be nullable?

@ajaych-ms @navali-msft

Fixed this.

minnieliu · 2021-06-18T02:49:14Z

+        public string SourcePhoneNumber => GetRecordedVariable(AlternateCallerId, options => options.IsSecret());

        /// <summary>
        /// The phone number associated with the source.


Should this be The phone number associated with the target.?

Good catch!
Change it to:
/// The phone number required to make a pstn call.

minnieliu · 2021-06-18T02:49:53Z

        /// The phone number associated with the source.
        /// </summary>
-        public string SourcePhoneNumber => GetRecordedVariable(AzurePhoneNumber, options => options.IsSecret());
+        public string SourcePhoneNumber => GetRecordedVariable(AlternateCallerId, options => options.IsSecret());


If we are going to change it to alternateCallerId, can we change the variable name as well?

We prefer to using the SourcePhoneNumber in comparasion with TargetPhoneNumber

Can you clarify why?

We have target phone number, so here we have source phone number, considering later on, the source phone number could be a pstn number that is not from ACS resource, then we assign another number and differentiate it with the AlternateCallerId from ACS.

zihzhan-msft · 2021-06-18T15:53:18Z

    /// </summary>
    public class CallingServerTestEnvironment : CommunicationTestEnvironment
    {
+        public const string AlternateCallerId = "ALTERNATE_CALLERID";


@minnieliu @RezaJooyandeh
Should we move this to CommunicationTestEnvironment.cs and create a Variable in the ACS Live Test KV?

We usually only add it to CommunicationTestEnvironment for variables that will be shared between all the Communication SDKs. I think it is fine to keep it in CallingServerTestEnvironment

JianpingChen · 2021-06-18T17:11:52Z

+namespace Azure.Communication.CallingServer
+{
+    [CodeGenModel("CommunicationErrorResponse")]
+    public partial class CallingServerErrorResponse


This class is not currently used, correct? If you plan to convert low level exceptions to this, you should only add this class when the conversion code is added. Otherwise, it is confusing

@navali-msft could you please remove

Removed the public version. Keep the one generated by sdk.

…operties to readonly (#21980) * Updated models definition based on new swagger, also fixed events to make properties readonly * Update accessibility of internal models * Update the file path in autorest

…o feature/callingServer-preview1-enhancement

RezaJooyandeh · 2021-06-21T20:17:26Z

    EnvVars:
      # SKIP_PHONENUMBER_LIVE_TESTS skips certain phone number tests such as purchase and release
-      SKIP_PHONENUMBER_LIVE_TESTS: TRUE
+      SKIP_PHONENUMBER_LIVE_TESTS: TRUE


why is SKIP_PHONENUMBER_LIVE_TESTS here?

I saw this flag is widely used in all the other communication SDK projects, even this flag is not used in them.

RezaJooyandeh · 2021-06-21T20:18:11Z

+
+using Azure.Core;
+
+namespace Azure.Communication.CallingServer


This is resolved.

RezaJooyandeh

I left another couple of small comments, but feel free to merge once you address those and @minnieliu approve as well.

zihzhan-msft and others added 3 commits June 7, 2021 23:47

Clean up clients comments.

3a31cac

Models updates to introduce CallConnection and Server.

2aac1b8

callState updated to callConnectionState, replace sourceAlternateIden…

89e1158

…tity with alternateCallerId, other fixes

ghost added the Communication label Jun 8, 2021

Update to the test sample.

2cdc4b8

navali-msft requested review from DominikMe, RezaJooyandeh and minnieliu June 8, 2021 16:51

zihzhan-msft reviewed Jun 8, 2021

View reviewed changes

navali-msft added 2 commits June 8, 2021 14:47

Updated the swagger path, removed delete operation, updated unit test…

8c07b7b

…s and live tests

Updates based on comments

e6322ed

ajaych-ms reviewed Jun 9, 2021

View reviewed changes

zihzhan-msft and others added 2 commits June 8, 2021 23:16

Merge branch 'feature/CallingServer-changes' into feature/callingServ…

09a1452

…er-preview1-enhancement

Zihzhan/fix (#21727)

bf54682

* Clean up clients comments. * Update Snippet and Export-API.

zihzhan-msft requested review from KrzysztofCwalina and tg-msft as code owners June 9, 2021 06:53

zihzhan-msft and others added 3 commits June 9, 2021 18:07

Merge branch 'feature/callingServer-preview1-enhancement' of https://…

f1efa58

…github.com/Azure/azure-sdk-for-net into feature/callingServer-preview1-enhancement

pull master without Downloader merge.

f3aebeb

Zihzhan/fix (#21764)

720d59c

* Update Snippet and Export-API. * remove calloption override in servercall client. * Nit for params and arguments handling. * Merge Downloader updates. * Merge Downloader updates test cases. * Update AutoGen Content.

RezaJooyandeh reviewed Jun 10, 2021

View reviewed changes

navali-msft and others added 8 commits June 10, 2021 15:11

Model name updates (Suffix for responses models now has Result instea…

0161219

…d of Response), fixes based on comments (#21790)

Fix build Anaylse.

37c39d4

fix(tests): improve code coverage

29ce1ed

Merge branch 'feature/callingServer-preview1-enhancement' of https://…

1717a69

…github.com/Azure/azure-sdk-for-net into feature/callingServer-preview1-enhancement

Added AddParticipantResult, Re-added communication error, updated swa…

20e4121

…gger path (#21870) * Added AddParticipantResult, Re-added communication error, updated swagger path * Fixed unit tests * Updated netstandard

Analysis fix

e829b98

zihzhan-msft added 2 commits June 16, 2021 23:11

RezaJooyandeh reviewed Jun 18, 2021

View reviewed changes

Comment thread sdk/communication/Azure.Communication.CallingServer/src/Models/CallingServerErrorResponse.cs Outdated

RezaJooyandeh reviewed Jun 18, 2021

View reviewed changes

Comment thread sdk/communication/Azure.Communication.CallingServer/src/Models/CallingServerError.cs Outdated

RezaJooyandeh reviewed Jun 18, 2021

View reviewed changes

minnieliu reviewed Jun 18, 2021

View reviewed changes

zihzhan-msft added 2 commits June 17, 2021 21:16

Address comments.

0a76c1a

Naming changes.

e264d03

zihzhan-msft reviewed Jun 18, 2021

View reviewed changes

JianpingChen reviewed Jun 18, 2021

View reviewed changes

zihzhan-msft added 2 commits June 18, 2021 12:45

Change to static connection string.

fbc6d87

Add methods that support Live Test.

b5d2464

navali-msft requested a review from benbp as a code owner June 18, 2021 23:54

zihzhan-msft and others added 6 commits June 18, 2021 17:17

Moved Recorded value inside the test method.

beaae94

Updated models definition based on new swagger plus updated events pr…

c5670ab

…operties to readonly (#21980) * Updated models definition based on new swagger, also fixed events to make properties readonly * Update accessibility of internal models * Update the file path in autorest

Update auto gen apis.

6d505c9

Merge branch 'main' of https://github.com/Azure/azure-sdk-for-net int…

072566e

…o feature/callingServer-preview1-enhancement

Rewrite the event tests and fixed json for other tests (#21983)

4d49521

Disable AddParticipant test in Live mode.

5b86515

RezaJooyandeh reviewed Jun 21, 2021

View reviewed changes

RezaJooyandeh approved these changes Jun 21, 2021

View reviewed changes

Moved modelfactory to .models namespace (#22021)

1efe0fd

minnieliu approved these changes Jun 21, 2021

View reviewed changes

minnieliu merged commit 658dbd4 into main Jun 21, 2021

minnieliu deleted the feature/callingServer-preview1-enhancement branch June 21, 2021 22:20

		@@ -46,9 +46,9 @@ protected async Task SleepIfNotInPlaybackModeAsync()
		await Task.Delay(10000);


		using Azure.Core;

		namespace Azure.Communication.CallingServer

Conversation

navali-msft commented Jun 8, 2021

All SDK Contribution checklist:

Additional management plane SDK specific contribution checklist:

Management plane SDK Troubleshooting

Old outstanding PR cleanup

Uh oh!

check-enforcer Bot commented Jun 8, 2021

What is Check Enforcer?

Why am I getting this message?

What should I do now?

What if I am onboarding a new service?

For data-plane/track 2 SDKs Issue the following command as a pull request comment:

For track 1 management-plane SDKs

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajaych-ms Jun 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajaych-ms Jun 9, 2021 •

edited

Loading